🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Floating-point conversion for Duffs is fragile.

UInt64(amountEachDash * 1e8) works for 0.001 (truncates 100000.000…002 to 100000), but breaks for other values — e.g. 0.0001 * 1e8 = 9999.999… truncates to 9999 instead of 10000. Define the amount in integer Duffs directly to avoid precision-dependent truncation.

-    private let amountEachDash: Double = 0.001
-    private var amountEachDuffs: UInt64 { UInt64(amountEachDash * 1e8) }
+    private let amountEachDuffs: UInt64 = 100_000 // 0.001 DASH
+    private var amountEachDash: Double { Double(amountEachDuffs) / 1e8 }

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift`
at line 13, Replace the floating-point amountEachDuffs calculation with an
integer Duffs constant or integer-based representation, and update
amountEachDash usage as needed so values such as 0.0001 map exactly to 10000
Duffs without floating-point multiplication or truncation.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Validate derived address count matches requested count.

If the FFI returns fewer addresses than count (e.g. null entries skipped by the if let), result will have fewer elements and the subsequent addresses[i] access in the funding loop will crash with an unhelpful index-out-of-bounds error. Add a post-loop assertion to fail fast with a clear message.

     for i in 0..<outCount {
         if let cstr = arr[i] {
             result.append(String(cString: cstr))
         }
     }
+    guard result.count == count else {
+        throw DerivationError(
+            description: "deriveExternalAddresses: expected \(count) addresses, got \(result.count)"
+        )
+    }
     return result

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift`
around lines 129 - 136, Add a post-loop assertion after the address collection
loop in the relevant address-derivation helper, verifying that result.count
equals outCount and reporting a clear failure message if not, before any
subsequent funding-loop indexing occurs.

🟡 Suggestion: Leaked FFIWallet from wallet_manager_get_wallet; ownership comment is factually wrong

I checked the pinned key-wallet-ffi source directly (rev 647fa9820f3614090e4e5f5f2b709961d68e538b, cached at ~/.cargo/git/checkouts/rust-dashcore-*/647fa98/key-wallet-ffi/src/wallet_manager.rs:371-388). wallet_manager_get_wallet clones the wallet from the manager (.cloned()) and returns a brand-new heap allocation every call: Box::into_raw(Box::new(FFIWallet::new(wallet))). Its doc comment is explicit: 'The returned wallet must be freed with wallet_free_const()', and wallet_free_const (wallet.rs:278-279) names this exact function by name as a caller of it. The manager does NOT retain ownership of this specific pointer.

The comment on line 101, 'Non-owning: the manager retains ownership of the wallet handle,' is incorrect, and there is no defer { wallet_free_const(wallet) } anywhere in deriveExternalAddresses. Every test run leaks one boxed FFIWallet. The same incorrect assumption already exists in production WalletManager.swift (confirmed: wallet_free_const is never called there despite 3 call sites of wallet_manager_get_wallet), so this is a pre-existing pattern rather than something this PR invented — but this PR adds a new call site with an incorrect rationale restated verbatim, and it's easy to fix here.

_{source: ['codex-ffi-engineer', 'sonnet5-ffi-engineer']}

🟡 Suggestion: "~20% synced" precondition is not actually guaranteed — it depends on prior tests' chain growth, not this test's own state

I traced the harness: IntegrationTestEnv.bootstrap() (IntegrationTestEnv.swift:113-165) runs exactly once per test process, syncs the SPV data dir to the chain tip at that moment via createSpvCache(), then snapshots it with snapshotSpvCache(). Every subsequent test's tearDown calls resetState() → restoreSpvCacheFromSnapshot() (lines 78-82, 223-241), which restores the working SPV dir to that same pre-synced snapshot before the next test runs. Meanwhile the underlying regtest node keeps mining blocks across the whole suite — it is never reset.

So when this test calls waitUntilUpToDate(height: tipHeight / 5), whether that wait does any real syncing work is entirely a function of how far the live chain has grown past the bootstrap-time snapshot height (i.e., how many blocks earlier tests in the same run happened to mine), not anything this test controls or asserts. waitUntilUpToDate (TestWallet.swift:69-89) returns as soon as cached headers/filters already meet the target — if the snapshot's cached height already exceeds tipHeight/5 (plausible after any funding-heavy tests ran first), this reduces to importing into an already-synced client, silently testing the same late-import scenario SpvLateWalletBackfillIntegrationTests already covers instead of a genuine mid-flight race. Nothing in the test verifies headers/filters are actually below the current chain tip before createWallet runs.

_{source: ['codex-general']}

🟡 Suggestion: Test name promises 'finds all tx' but only aggregate balance is asserted, not transaction history

The test method is testManyTxImportedMidSyncBackfillsAllHistory and the PR title says 'finds all tx', but the only assertion (lines 65-69) checks imported.balance().total == expectedTotal. Balance/UTXO tracking and transaction-history persistence are separate write paths — PlatformWalletPersistenceHandler independently writes rows into PersistentTransaction. A regression that restores the correct aggregate balance while dropping one or more of the 10 individual transaction records from history would pass this test undetected, since the 10 funding txids are never captured or checked against the imported wallet's history.

_{source: ['codex-general']}

🟡 Suggestion: Funding loop bypasses env.fund()'s InstantSend/masternode-broadcast steps

I confirmed env.fund(address:dash:) (IntegrationTestEnv.swift:391-398) does sendToAddress → broadcastToMasternodes(txid:) → waitForInstantSendLock(txid:) → mine(1), and every other test in this suite (SpvLateWalletBackfillIntegrationTests, CoreSendIntegrationTests, SpvRestartIntegrationTests, etc.) uses this helper. This new test instead calls env.coreRPC.sendToAddress directly and immediately env.mine(1), skipping the masternode broadcast and InstantSend-lock wait. Since this test funds 10 separate addresses across 10 blocks in quick succession, it's the test most likely to expose the absence of that synchronization the rest of the suite already guards against, making it a plausible source of CI flakiness.

_{source: ['sonnet5-general']}

💬 Nitpick: Comment is truncated mid-sentence

"Wait until the wallet is fully synced, then check that the balance is" cuts off before completing the thought. Confirmed verbatim in the file.

_{source: ['sonnet5-general']}

💬 Nitpick: No guard that deriveExternalAddresses returned exactly numTxs addresses before indexing

deriveExternalAddresses skips any null C-string entries when building result (line 132: if let cstr = arr[i]), so it could in principle return fewer than count addresses. The funding loop then indexes addresses[i] for i in 0..<numTxs unconditionally, which would crash with an out-of-range index rather than a clear assertion failure if that ever happened. Low likelihood given the FFI contract, but a cheap XCTAssertEqual(addresses.count, numTxs) before the loop would fail more legibly.

_{source: ['sonnet5-general']}